Conversation
…ialization The OrganizationIntegrationSerializer could produce null entries in API responses when per-item serialization failed. The _serialize catch-all in base.py swallowed exceptions and returned None, which was included directly in the response list, crashing the frontend. - Use .get() in get_attrs to handle missing Integration records gracefully instead of KeyError-ing the entire request - Add null guards for missing integration and failed inner serialization - Catch broad exceptions in get_installation and installation methods with specific log messages instead of relying on the generic _serialize catch-all - Filter null items from both organization-integrations endpoints Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ation Revert custom exception handling in OrganizationIntegrationSerializer and instead rely on the existing _serialize catch-all in base.py to log failures. Add exception detail to the base serializer log for better debugging. Update tests to match simplified approach. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ging Remove the exception object from logger extras to avoid logging sensitive data (secrets, passwords) in cleartext, as flagged by CodeQL. The exception traceback is still captured by logger.exception and sentry_sdk.capture_exception. Co-Authored-By: Claude Opus 4.6 <noreply@example.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Test doesn't verify None-filtering due to CASCADE delete
- Fixed test to mock get_integrations returning empty list, which triggers KeyError in serializer causing exception handling to return None as intended.
- ✅ Fixed: Duplicate Sentry events from exception and logger
- Removed redundant sentry_sdk.capture_exception call since logger.exception already sends to Sentry via EventHandler.
Or push these changes by commenting:
@cursor push 9786d13de3
Preview (9786d13de3)
diff --git a/src/sentry/api/serializers/base.py b/src/sentry/api/serializers/base.py
--- a/src/sentry/api/serializers/base.py
+++ b/src/sentry/api/serializers/base.py
@@ -117,9 +117,8 @@
) -> Mapping[str, Any] | None:
try:
return self.serialize(obj, attrs, user, **kwargs)
- except Exception as e:
+ except Exception:
logger.exception("Failed to serialize", extra={"instance": obj})
- sentry_sdk.capture_exception(e)
return None
def serialize(
diff --git a/tests/sentry/integrations/api/endpoints/test_user_organizationintegration.py b/tests/sentry/integrations/api/endpoints/test_user_organizationintegration.py
--- a/tests/sentry/integrations/api/endpoints/test_user_organizationintegration.py
+++ b/tests/sentry/integrations/api/endpoints/test_user_organizationintegration.py
@@ -2,7 +2,6 @@
import orjson
-from sentry.integrations.models.integration import Integration
from sentry.testutils.cases import APITestCase
from sentry.testutils.silo import control_silo_test
@@ -75,16 +74,18 @@
assert content == []
def test_missing_integration_filtered_from_response(self) -> None:
- """OrganizationIntegrations whose Integration was deleted don't produce null entries."""
+ """OrganizationIntegrations whose Integration is missing don't produce null entries."""
integration = self.create_provider_integration(provider="github")
self.create_organization_integration(
organization_id=self.organization.id, integration_id=integration.id
)
- # Delete the Integration row directly to simulate orphaned state
- Integration.objects.filter(id=integration.id).delete()
+ with patch(
+ "sentry.integrations.services.integration.impl.get_integrations",
+ return_value=[],
+ ):
+ response = self.get_success_response(self.user.id)
- response = self.get_success_response(self.user.id)
assert response.data == []
def test_serialization_error_filtered_from_response(self) -> None:This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
Reviewed by Cursor Bugbot for commit dc7fb9b. Configure here.
| request=request, | ||
| queryset=queryset, | ||
| on_results=lambda x: serialize(x, request.user), | ||
| on_results=lambda x: [item for item in serialize(x, request.user) if item is not None], |
There was a problem hiding this comment.
Bug: The get_attrs method in OrganizationIntegrationSerializer still uses direct dictionary access, which will cause a KeyError if an Integration record is missing, despite the PR's intent.
Severity: HIGH
Suggested Fix
In src/sentry/integrations/api/serializers/models/integration.py, modify the get_attrs method to use integrations_by_id.get(item.integration_id) instead of integrations_by_id[item.integration_id]. This will prevent a KeyError when an integration is not found, allowing the serialization process to handle the missing item gracefully.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/sentry/api/endpoints/user_organizationintegrations.py#L56
Potential issue: The pull request aims to prevent a server error when an
`OrganizationIntegration` record points to a deleted `Integration`. However, the fix is
incomplete. The `OrganizationIntegrationSerializer.get_attrs` method still uses direct
dictionary access (`integrations_by_id[item.integration_id]`) instead of a safe method
like `.get()`. If an integration has been deleted,
`integration_service.get_integrations` will not return it, causing a `KeyError` when
`get_attrs` tries to access the missing ID. This error occurs before the per-item
serialization loop, so the filtering logic added in the API endpoints is never reached,
and the entire API request will fail with a 500 error.
Did we get this right? 👍 / 👎 to inform future reviews.
The test_missing_integration_filtered_from_response test never exercised the None-filtering logic it claimed to test. The Integration foreign key uses CASCADE on_delete, so deleting the Integration also cascade-deleted the OrganizationIntegration row, leaving zero results for the queryset. Even bypassing CASCADE, the queryset's integration__status JOIN would exclude orphaned rows. The test passed vacuously regardless of whether the fix existed. The remaining test_serialization_error_filtered_from_response properly exercises the None-filtering by triggering a real exception in _serialize. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
wedamija
left a comment
There was a problem hiding this comment.
I don't really know if I like this pattern of just filtering out when serializers fail, it means we return incorrect data to users. But it's already embedded in our system now so this if fine for the moment.
Tbh, we probably shouldn't be catching serializer errors and replacing them with None, but that's a bigger fix to get through
…ialization (#112391) some integrations failed to serialize, causing `null`s to appear in the list apis: <img width="1380" height="208" alt="image" src="https://github.com/user-attachments/assets/1c90c40d-edd1-43ff-bcae-930bc4be9d9d" /> this breaks frontend type assertions which necessitated the patch in #112368 to avoid the page crashing [the logs in sentry](https://sentry.sentry.io/issues/7384876727/?project=11276#logs) confirm that there were entries that failed to serialize here: <img width="473" height="90" alt="image" src="https://github.com/user-attachments/assets/3733161c-5df1-4907-830b-1bc262476bba" /> This fix will omit `None` entries from the response, so the frontend can be reset to being type safe, and add some more logging we can look into about why certain bad integrations can't be serialized. +tests --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Claude Opus 4.6 <noreply@example.com>


some integrations failed to serialize, causing
nulls to appear in the list apis:this breaks frontend type assertions which necessitated the patch in #112368
to avoid the page crashing
the logs in sentry confirm that there were entries that failed to serialize here:
This fix will omit
Noneentries from the response, so the frontend can be reset to being type safe, and add some more logging we can look into about why certain bad integrations can't be serialized. +tests